Minor fixes for eslint.codeActionsOnSave.rules mechanism #1364
Conversation
When adding new entries to saveRuleConfigCache those entries are indexed by file path (lines 589 and 592), but we are trying to delete them by URI. This is causing cache entries to become stale if the settings are changed.
The function getSaveRuleConfig returns undefined on eslint@8 unless the setting eslint.useESLintClass == true, but there's no point on using this setting with eslint@8 as the CLIEngine is no longer available. To fix this I changed the condition to check if the ESLintClass.isCliEngine == true, this way we can keep the logic about the engine selection and capabilities only within the ESLintClass.newESLintClass function.
|
@edupsousa thanks for the PR. One change I would like to request: use also the URI for the |
| async function getSaveRuleConfig(filePath: string, settings: TextDocumentSettings & { library: ESLintModule }): Promise<SaveRuleConfigItem | undefined> { | ||
| let result = saveRuleConfigCache.get(filePath); | ||
| if (result === null) { | ||
| async function getSaveRuleConfig(uri: string, settings: TextDocumentSettings & { library: ESLintModule }): Promise<SaveRuleConfigItem | undefined> { |
There was a problem hiding this comment.
Replaced filePath argument by the uri, as it is need here to get and set items on saveRuleConfigCache. Preferred this way instead of adding the uri to the arguments list to keep the function signature concise.
| async function getSaveRuleConfig(uri: string, settings: TextDocumentSettings & { library: ESLintModule }): Promise<SaveRuleConfigItem | undefined> { | ||
| const filePath = getFilePath(uri); | ||
| let result = saveRuleConfigCache.get(uri); | ||
| if (filePath === undefined || result === null) { |
There was a problem hiding this comment.
filePath still required here to be used with ESLintClass.calculateConfigForFile method, so if the uri schema couldn't be resolved to a file path we return undefined in the same way of when there's no rules.
| @@ -2283,7 +2283,7 @@ async function computeAllFixes(identifier: VersionedTextDocumentIdentifier, mode | |||
| connection.tracer.log(`Computing all fixes took: ${Date.now() - start} ms.`); | |||
| return result; | |||
| } else { | |||
| const saveConfig = filePath !== undefined && mode === AllFixesMode.onSave ? await getSaveRuleConfig(filePath, settings) : undefined; | |||
| const saveConfig = filePath !== undefined && mode === AllFixesMode.onSave ? await getSaveRuleConfig(uri, settings) : undefined; | |||
There was a problem hiding this comment.
This is the only call to getSaveRuleConfig function, so I changed the call to use the uri instead of the filePath.
As a personal decision I kept the filePath !== undefined condition, even if we are not using filePath in the call anymore. I thought that it could be more efficient by avoiding an asynchronous call if we already know its return value, but I'm not really sure.
|
@dbaeumer did the request changes to keep |
|
Looks good to me. |
|
@dbaeumer I have some spare time and would love to continue contributing to the project, do you want to point me to any specific issue that needs help with coding? |
|
@edupsousa great. If you want you can have a look at: #1206 If you need help simply ping me in the issue |
Hi @dbaeumer,
I was studying a way to contribute with #1357 and stepped on two small issues: a bug leading to stale entries on
saveRuleConfigCacheobject, and behavior different from the documentation when usingeslint.codeActionsOnSave.ruleswith ESLint version 8.The stale cache is being caused by the use of the file URI to delete cache entries when a file is closed, those entries are indexed by the file path, not the URI. This fix only gets the file path from the URI and uses it to delete the cache entries, it was the simplest solution, but maybe using the URI to index cache entries could be a better one?
The behavior problem is because the function
getSaveRuleConfigalways returns undefined ifeslint.useESLintClass == false, this makes sense to ESLint version 7, but not for version 8 where the class is the only engine available. This fix removes the responsibility of checking whether the class is available or theeslint.useESLintClassfrom the functiongetSaveRuleConfig, and decides if the rules should not be processed by checking ifESLintClass.isCLIEngine == true.Hope that helps,
Eduardo